fix(v9/mcp): capture prompt results and add defensive edge cases for transport instrumentation #17401
Conversation
…calls (#17284) closes #17283 includes these attributes for `mcp.server` spans: - `mcp.prompt.result.description` - `mcp.prompt.result.message_content` - `mcp.prompt.result.message_role` - `mcp.prompt.result.message_count` Example: <img width="835" height="300" alt="Screenshot 2025-08-01 at 12 40 46" src="https://github.com/user-attachments/assets/592d876b-807a-4f3e-a9b2-406e10f5a83d" /> Needed to make `attributeExtraction.ts` <300 lines of code (requirement) so it's now split between `sessionExtraction.ts`, `sessionExtraction.ts` and `resultExtraction.ts`. So changes explained so it's easier to review: - The only function this PR adds is `extractPromptResultAttributes` inside `resultExtraction.ts`. - It adds the prompt results as PII in `piiFiltering.ts`. Just add them to the `set`. - adds a `else if (method === 'prompts/get')` to execute the `extractPromptResultAttributes` function. - adds a test that checks we're capturing the results and updates the PII test to check PII result attributes are being removed if sending PII is not enabled. (cherry picked from commit 0e05a40)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@sentry review |
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| */ | ||
| function buildAllContentItemAttributes(content: unknown[]): Record<string, string | number | boolean> { | ||
| const attributes: Record<string, string | number> = { |
There was a problem hiding this comment.
The return type annotation is inconsistent. The function signature declares Record<string, string | number | boolean> but the implementation declares attributes as Record<string, string | number> and then returns it. This creates a type mismatch where boolean values could be assigned but the internal type doesn't account for them.
Did we get this right? 👍 / 👎 to inform future reviews.
| if (!isValidContentItem(message)) { | ||
| continue; | ||
| } | ||
|
|
||
| const prefix = messages.length === 1 ? 'mcp.prompt.result' : `mcp.prompt.result.${i}`; | ||
|
|
||
| const safeSet = (key: string, value: unknown): void => { | ||
| if (typeof value === 'string') { |
There was a problem hiding this comment.
The attribute naming logic in extractPromptResultAttributes is complex and could be error-prone. The conditional logic for single vs multiple messages creates different attribute names (message_${key} vs just ${key}) which might be confusing for consumers. Consider using a more consistent naming scheme.
Did we get this right? 👍 / 👎 to inform future reviews.
| if (key.startsWith(`${MCP_TOOL_RESULT_PREFIX}.`) || key.startsWith(`${MCP_PROMPT_RESULT_PREFIX}.`)) { | ||
| if (!key.endsWith('_count') && !key.endsWith('_error') && !key.endsWith('.is_error')) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The PII filtering logic has inconsistent handling of metadata attributes. The code preserves attributes ending with _count, _error, or .is_error, but the test file shows attributes like mcp.resource.result.content and mcp.resource.result.uri which don't fall under tool/prompt result prefixes, potentially creating gaps in PII protection.
Did we get this right? 👍 / 👎 to inform future reviews.
| * @returns Transport type mapping for span attributes | ||
| */ | ||
| export function getTransportTypes(transport: MCPTransport): { mcpTransport: string; networkTransport: string } { | ||
| const transportName = transport.constructor?.name?.toLowerCase() || ''; | ||
|
|
||
| if (transportName.includes('stdio')) { | ||
| return { mcpTransport: 'stdio', networkTransport: 'pipe' }; | ||
| } | ||
|
|
||
| if (transportName.includes('streamablehttp') || transportName.includes('streamable')) { | ||
| return { mcpTransport: 'http', networkTransport: 'tcp' }; | ||
| } | ||
|
|
||
| if (transportName.includes('sse')) { | ||
| return { mcpTransport: 'sse', networkTransport: 'tcp' }; | ||
| } | ||
|
|
||
| return { mcpTransport: 'unknown', networkTransport: 'unknown' }; |
There was a problem hiding this comment.
The getTransportTypes function relies on constructor name matching which is fragile and could break with minification or different class naming conventions. Consider using more robust type detection methods or documenting the dependency on specific constructor naming.
Did we get this right? 👍 / 👎 to inform future reviews.
| } else if (method === 'prompts/get') { | ||
| const rawPromptAttributes = extractPromptResultAttributes(result); | ||
| const client = getClient(); | ||
| const sendDefaultPii = Boolean(client?.getOptions().sendDefaultPii); | ||
| const promptAttributes = filterMcpPiiFromSpanData(rawPromptAttributes, sendDefaultPii); | ||
|
|
||
| span.setAttributes(promptAttributes); |
There was a problem hiding this comment.
The new prompt result attribute extraction is missing error handling. If extractPromptResultAttributes throws an exception, it could cause the span completion to fail silently. Consider adding try-catch blocks similar to how other result extractions might be handled.
Did we get this right? 👍 / 👎 to inform future reviews.
backport of #17284 and #17291